feat(wallet): explicit wallet.restore() recovery mechanism#492
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (17)
WalkthroughThis PR implements HD wallet contract recovery scanning by introducing a new ChangesHD Wallet Restore and Contract Discovery
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: feat(wallet): explicit wallet.restore() recovery mechanism
Verdict: Request changes (protocol-critical — requires human sign-off)
This is well-architected fund-recovery code with solid error semantics and thorough test coverage. The gap-limit scan, error-collection model, and concurrency guard are all sound. However, this is protocol-critical (VTXO discovery = fund recovery) — bugs here make users believe they have no funds when they do. Requesting human review per repo policy.
🟢 Strengths
- Error contract is correct: handler
discoverAterrors collected → scan finishes → inline VTXO pull recovers safely-discovered funds → thenAggregateErrorsurfaces. A truncated restore is loud, never silent. - Concurrency model is solid:
_restoreInFlightcoalesces concurrent calls;dispose()drains it deadlock-free (restore never calls dispose). - HD watermark is monotonic:
advanceLastIndexUsedusesmutate()(mutex-protected) and only advances. - Cross-repo impact is nil: all changes are additive. No external implementations of
IContractManagerexist. ThematerializeAt→materializeDescriptorAtrename is internal. - Test coverage: 28 unit cases + e2e. Key invariants tested: gap-window-kept-open-by-swap, concurrent coalesce, dispose-races-restore, handler-error-after-pull.
🟡 Minor Issues (non-blocking, but worth addressing)
1. Unbounded loop if a buggy handler always returns results
src/contracts/contractManager.ts:131 — while (i <= maxIdx && unused < gapLimit) with maxIdx = POSITIVE_INFINITY for HD mode. If a malicious/buggy third-party Discoverable handler always returns non-empty from discoverAt, the loop never terminates.
Suggestion: Add a hard ceiling (e.g. const MAX_INDEX = 10_000) as a safety valve. Gap-limit 20 with continuous hits reaching 10k indices would already be extraordinary; an infinite loop would OOM/hang the wallet.
const maxIdx = opts.hd ? MAX_INDEX : 0;2. Sequential handler probing — potential performance cliff
src/contracts/contractManager.ts:135-145 — handlers are iterated sequentially within each index. With N handlers × G gap-limit × T timelocks per handler, each making an indexer round-trip, a restore with 3 handlers and gap 20 would make ~60+ serial network calls minimum.
Suggestion (future): Consider Promise.all across handlers within an index (they're independent). Not blocking this PR, but worth a TODO comment.
3. Duck-typing for HD detection is fragile
src/wallet/wallet.ts:1192-1194:
const hd = !!provider && typeof (provider as Partial<HDDescriptorProvider>).materializeDescriptorAt === "function";Any non-HD DescriptorProvider that happens to have a materializeDescriptorAt method would be incorrectly treated as HD. Consider using instanceof HDDescriptorProvider (you already import it) for type safety:
const hd = provider instanceof HDDescriptorProvider;🟢 No Issues Found In
- Gap-limit termination logic (correct BIP44-style scan)
createContractidempotency reliance (script-keyed dedup is existing behavior)- Metadata tagging (index 0 untagged, index > 0 tagged with
wallet-receive+signingDescriptor) deriveDescriptorLeafPubKeyextraction (logic identical to original)WALLET_RECEIVE_SOURCElayering fix (re-export maintains back-compat)pickActiveReceivetiebreak (deterministic on HD index)- ServiceWorker proxy rejection (clear error, correct — callbacks not structured-cloneable)
signingDescriptorIndexregex (correctly parses trailing/N)pattern)
⚠️ Protocol-Critical Flag
This PR adds fund-recovery logic. If scanContracts terminates early (gap closes before all funds discovered), or if refreshVtxos fails silently after the scan, users lose visibility of their own money. The error contract handles both cases correctly in the current implementation, but this requires human sign-off before merge per protocol-critical review policy.
TL;DR: Clean, well-tested, correct error semantics. The unbounded-loop ceiling (#1) should be addressed before merge; #2-#3 are nice-to-haves. Human approval required for protocol-critical code.
There was a problem hiding this comment.
Review: feat(wallet): explicit wallet.restore() recovery mechanism
Verdict: Request changes (protocol-critical — requires human sign-off)
This is well-architected fund-recovery code with solid error semantics and thorough test coverage. The gap-limit scan, error-collection model, and concurrency guard are all sound. However, this is protocol-critical (VTXO discovery = fund recovery) — bugs here make users believe they have no funds when they do. Requesting human review per repo policy.
🟢 Strengths
- Error contract is correct: handler
discoverAterrors collected → scan finishes → inline VTXO pull recovers safely-discovered funds → thenAggregateErrorsurfaces. A truncated restore is loud, never silent. - Concurrency model is solid:
_restoreInFlightcoalesces concurrent calls;dispose()drains it deadlock-free (restore never calls dispose). - HD watermark is monotonic:
advanceLastIndexUsedusesmutate()(mutex-protected) and only advances. - Cross-repo impact is nil: all changes are additive. No external implementations of
IContractManagerexist. ThematerializeAt→materializeDescriptorAtrename is internal. - Test coverage: 28 unit cases + e2e. Key invariants tested: gap-window-kept-open-by-swap, concurrent coalesce, dispose-races-restore, handler-error-after-pull.
🟡 Minor Issues (non-blocking, but worth addressing)
1. Unbounded loop if a buggy handler always returns results
src/contracts/contractManager.ts:131 — while (i <= maxIdx && unused < gapLimit) with maxIdx = POSITIVE_INFINITY for HD mode. If a malicious/buggy third-party Discoverable handler always returns non-empty from discoverAt, the loop never terminates.
Suggestion: Add a hard ceiling (e.g. const MAX_INDEX = 10_000) as a safety valve. Gap-limit 20 with continuous hits reaching 10k indices would already be extraordinary; an infinite loop would OOM/hang the wallet.
const maxIdx = opts.hd ? MAX_INDEX : 0;2. Sequential handler probing — potential performance cliff
src/contracts/contractManager.ts:135-145 — handlers are iterated sequentially within each index. With N handlers × G gap-limit × T timelocks per handler, each making an indexer round-trip, a restore with 3 handlers and gap 20 would make ~60+ serial network calls minimum.
Suggestion (future): Consider Promise.all across handlers within an index (they're independent). Not blocking this PR, but worth a TODO comment.
3. Duck-typing for HD detection is fragile
src/wallet/wallet.ts:1192-1194:
const hd = !!provider && typeof (provider as Partial<HDDescriptorProvider>).materializeDescriptorAt === "function";Any non-HD DescriptorProvider that happens to have a materializeDescriptorAt method would be incorrectly treated as HD. Consider using instanceof HDDescriptorProvider (you already import it) for type safety:
const hd = provider instanceof HDDescriptorProvider;🟢 No Issues Found In
- Gap-limit termination logic (correct BIP44-style scan)
createContractidempotency reliance (script-keyed dedup is existing behavior)- Metadata tagging (index 0 untagged, index > 0 tagged with
wallet-receive+signingDescriptor) deriveDescriptorLeafPubKeyextraction (logic identical to original)WALLET_RECEIVE_SOURCElayering fix (re-export maintains back-compat)pickActiveReceivetiebreak (deterministic on HD index)- ServiceWorker proxy rejection (clear error, correct — callbacks not structured-cloneable)
signingDescriptorIndexregex (correctly parses trailing/N)pattern)
⚠️ Protocol-Critical Flag
This PR adds fund-recovery logic. If scanContracts terminates early (gap closes before all funds discovered), or if refreshVtxos fails silently after the scan, users lose visibility of their own money. The error contract handles both cases correctly in the current implementation, but this requires human sign-off before merge per protocol-critical review policy.
TL;DR: Clean, well-tested, correct error semantics. The unbounded-loop ceiling (#1) should be addressed before merge; #2-#3 are nice-to-haves. Human approval required for protocol-critical code.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
test/restore.test.ts (2)
426-468: ⚡ Quick winAssert that the scan really continues past the hit.
This regression test says the hit at index 4 keeps the window open, but the current assertions only prove the hit was found and
lastIndexUsedbecame 4. A buggy loop that stops immediately after the first hit would still pass. CapturecallsfrommakeFakeHandler()and assert the handler was probed through the full post-hit window.♻️ Suggested assertion
- const { handler } = makeFakeHandler("swapfake", (i) => + const { handler, calls } = makeFakeHandler("swapfake", (i) => i === 4 ? [ { type: "swapfake", params: { script: "aabb" }, @@ // so the loop kept probing 5..9 instead of stopping at 4, and // lastIndexUsed is driven solely by the swap handler. expect(res.lastIndexUsed).toBe(4); expect(res.handlerErrors).toEqual([]); + expect(calls).toEqual([0, 1, 2, 3, 4, 5, 6, 7, 8, 9]); // The contract was actually registered (idempotent createContract). const [c] = await mgr.getContracts({ script: "aabb" }); expect(c?.type).toBe("swapfake");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/restore.test.ts` around lines 426 - 468, The test currently only asserts that a swap hit at index 4 was found and lastIndexUsed is 4, but doesn't verify the scanner actually probed indices after the hit; capture the handler invocation details returned by makeFakeHandler (the `handler`/`calls` structure) and add an assertion that the handler was invoked for indices beyond 4 up to the expected post-hit window (given gapLimit:5), e.g., verify `calls` includes probes for indices 5..(4 + gapLimit - 1) or the equivalent sequence, so the test fails if the loop stopped immediately after the hit; update the test to extract `calls` from makeFakeHandler and assert the expected indices were probed.
102-111: ⚡ Quick winCover the partial-hit timelock case.
mockIndexer()reports success when any queried script matches, and both multi-timelock tests only cover the “all scripts hit” path. A handler regression that batches timelocks together and treats one hit as evidence for every candidate would still pass here. Please add a case where only one timelock script exists, or make the mock return per-script hits so the suite can catch false positives.Also applies to: 174-220, 302-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/restore.test.ts` around lines 102 - 111, The mockIndexer’s getVtxos currently returns a single boolean hit if any queried script matches, which masks partial-hit bugs; update mockIndexer (and tests at the other locations) so getVtxos returns per-script results (or at least an explicit case where only one timelock script is present) — e.g., have mockIndexer examine each script in opts.scripts and return vtxos only for the scripts that are in usedScripts (or add a new test that supplies two timelock scripts but only marks one as used and asserts the handler does not treat both as found); target the mockIndexer function and getVtxos behavior used by the multi-timelock tests to ensure partial-hit cases are covered.test/e2e/restore.test.ts (1)
22-59: ⚡ Quick winAlways dispose the e2e wallets in
finally.If any assertion fails before the happy-path disposals, these wallet instances stay alive and can leak repo/indexer state into later e2e runs. Please wrap both wallet lifecycles in
try/finallyso cleanup is guaranteed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/restore.test.ts` around lines 22 - 59, The test creates two wallets via createTestArkWalletFromMnemonic (variables a and b) but only disposes them on the happy path; move both lifecycles into try/finally blocks so each wallet is always disposed even if assertions fail: surround the logic after creating a with try { ... } finally { await a.wallet.dispose(); } and likewise surround the logic after creating b (using createSharedRepos and calling faucetOffchain/waitFor/getBalance/restore) with a try/finally that awaits b.wallet.dispose(); ensure awaits remain and no test logic is left outside the try so cleanup is guaranteed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/contracts/contractManager.ts`:
- Around line 528-537: The discovery loop currently calls createContract() which
immediately triggers full hydration via fetchContractVxosFromIndexer() and
watcher registration; to avoid N full indexer pulls during restore, add a new
lightweight API (e.g., persistAndWatchContract or createContractWatchOnly) on
the same class that only persists the DiscoveredContract and registers watchers
but does not call fetchContractVxosFromIndexer(), then replace the
createContract(c) call inside the discoverables loop with this new method; keep
createContract(c) intact for later restore flow where refreshVtxos({
includeInactive: true }) will perform the full hydration. Ensure the new method
signature accepts the DiscoveredContract and any opts needed and that any error
handling (handlerErrors) remains unchanged.
In `@src/wallet/hdDescriptorProvider.ts`:
- Around line 134-142: The advanceLastIndexUsed method currently accepts invalid
watermarks; before calling mutate() add a guard that validates the incoming
index using Number.isInteger(index) && index >= 0 and bail out (no-op) if it
fails so you never persist NaN, fractions, or negatives to
settings.lastIndexUsed; ensure you reference advanceLastIndexUsed, mutate, and
settings.lastIndexUsed in the change and note that invalid values currently
break parseSettings on subsequent reads.
In `@src/wallet/wallet.ts`:
- Around line 1229-1243: The current HD branch uses hd inferred only from
materializeDescriptorAt but later calls advanceLastIndexUsed, causing a
TypeError; update the capability check to require both functions
(materializeDescriptorAt and advanceLastIndexUsed) on this._descriptorProvider
(e.g., replace hd with hdCapable or similar), narrow the type to an
HDDescriptorProvider-like shape for the rest of _runRestore, and adjust the
materialize function and subsequent calls (including where advanceLastIndexUsed
is invoked) to rely on that guarded, correctly typed provider so custom
providers that only implement one method won't take the HD path.
- Around line 1213-1224: Move the early-return that checks this._restoreInFlight
to before any validation of opts so concurrent calls coalesce; specifically, in
restore() check if (this._restoreInFlight) return this._restoreInFlight; first,
then compute/validate gapLimit and set this._restoreInFlight =
this._runRestore(gapLimit).finally(...). Ensure you still validate gapLimit
(Number.isInteger and >0) only for the caller that actually starts the run, and
keep the same error message when throwing.
In `@test/helpers/restoreWallet.ts`:
- Around line 84-95: makeVtxo currently returns a constant outpoint (txid/vout)
which causes accidental deduping; update makeVtxo to generate a unique outpoint
per call (e.g., derive txid or vout from the script, a counter, timestamp, or a
short hash of script+Date.now()) so each VirtualCoin has a distinct txid/vout
pair while preserving other fields (value, status, createdAt, script,
isUnrolled, isSpent, virtualStatus). Apply the same uniqueness fix to the other
similar helper(s) referenced in the file (the alternate makeVtxo usage around
the later lines 125-127) so all mocked VTXOs use unique outpoints.
- Around line 259-261: The current unchecked cast retrieving hdProvider from
wallet (const hdProvider = (wallet as unknown as { _descriptorProvider:
HDDescriptorProvider })._descriptorProvider) should be protected by a runtime
guard: verify wallet._descriptorProvider exists and matches the expected shape
(e.g., instanceof HDDescriptorProvider or has required methods/properties) and
if not throw a clear, fast-failing error mentioning wallet and
HDDescriptorProvider; replace the direct cast with this guard and use the
validated hdProvider thereafter so failures are explicit if wallet internals
change.
---
Nitpick comments:
In `@test/e2e/restore.test.ts`:
- Around line 22-59: The test creates two wallets via
createTestArkWalletFromMnemonic (variables a and b) but only disposes them on
the happy path; move both lifecycles into try/finally blocks so each wallet is
always disposed even if assertions fail: surround the logic after creating a
with try { ... } finally { await a.wallet.dispose(); } and likewise surround the
logic after creating b (using createSharedRepos and calling
faucetOffchain/waitFor/getBalance/restore) with a try/finally that awaits
b.wallet.dispose(); ensure awaits remain and no test logic is left outside the
try so cleanup is guaranteed.
In `@test/restore.test.ts`:
- Around line 426-468: The test currently only asserts that a swap hit at index
4 was found and lastIndexUsed is 4, but doesn't verify the scanner actually
probed indices after the hit; capture the handler invocation details returned by
makeFakeHandler (the `handler`/`calls` structure) and add an assertion that the
handler was invoked for indices beyond 4 up to the expected post-hit window
(given gapLimit:5), e.g., verify `calls` includes probes for indices 5..(4 +
gapLimit - 1) or the equivalent sequence, so the test fails if the loop stopped
immediately after the hit; update the test to extract `calls` from
makeFakeHandler and assert the expected indices were probed.
- Around line 102-111: The mockIndexer’s getVtxos currently returns a single
boolean hit if any queried script matches, which masks partial-hit bugs; update
mockIndexer (and tests at the other locations) so getVtxos returns per-script
results (or at least an explicit case where only one timelock script is present)
— e.g., have mockIndexer examine each script in opts.scripts and return vtxos
only for the scripts that are in usedScripts (or add a new test that supplies
two timelock scripts but only marks one as used and asserts the handler does not
treat both as found); target the mockIndexer function and getVtxos behavior used
by the multi-timelock tests to ensure partial-hit cases are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 310ef6b3-7dac-4e9d-a7b5-480fbccaa359
📒 Files selected for processing (18)
src/contracts/contractManager.tssrc/contracts/handlers/default.tssrc/contracts/handlers/delegate.tssrc/contracts/index.tssrc/contracts/metadata.tssrc/contracts/types.tssrc/identity/descriptor.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/serviceWorker/wallet.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/e2e/restore.test.tstest/e2e/utils.tstest/helpers/hdProvider.tstest/helpers/restoreWallet.tstest/helpers/scanManager.tstest/restore.test.ts
There was a problem hiding this comment.
Follow-up review (commit 0aaac20)
New commit: test(e2e): make restore test HD-mode and load-bearing — test-only, no production code changed.
✅ New test is correct and well-designed
The old e2e test was vacuous — index-0 funds are auto-registered at baseline, so a same-seed wallet B would see them without restore(). The new test is genuinely load-bearing:
- Funds wallet A at index-0 → receive rotator advances to index-1
- Funds wallet A at index-1 (a script baseline doesn't cover)
- Fresh wallet B on same seed → baseline only sees index-0 →
before < totalA - After
restore()→ gap scan discovers index-1 →after >= totalA
The waitFor polling for rotation and the toBeLessThan assertion (instead of === 0) correctly handle the timing window where B's watcher may or may not have synced index-0 VTXOs. Good.
The utils.ts change (adding optional walletMode param) is clean — no default changed, existing callers unaffected.
⚠️ Previous review concerns still open
My prior review's findings remain unaddressed:
- Unbounded loop (
contractManager.ts:518—maxIdx = POSITIVE_INFINITY) — still needs a hard ceiling - Duck-typing for HD detection (
wallet.ts:1233) — still uses method-existence check instead ofinstanceof
These are non-blocking for this test commit but still need resolution before merge. Protocol-critical flag and human sign-off requirement remain.
…ndexUsed on HDDescriptorProvider
Add the Discoverable capability to DefaultContractHandler so it participates in wallet.restore()'s gap-limit scan. discoverAt probes every csvTimelock in the baseline matrix at the given descriptor's leaf pubkey and returns any contracts the indexer shows on-chain history for. Index 0 produces an untagged DiscoveredContract; index > 0 tags with wallet-receive source + signingDescriptor metadata. Type annotation widened to ContractHandler & Discoverable.
…→wallet cycle Move the source-of-truth declaration of WALLET_RECEIVE_SOURCE to the new dependency-free leaf src/contracts/metadata.ts. Update contracts/handlers/default.ts to import from ../metadata (dropping the ../../wallet/walletReceiveRotator import that caused the cycle). Add a re-export from wallet/walletReceiveRotator for backward compatibility.
…asts Add a test that calls discoverAt with two distinct csvTimelocks, asserts both are returned as separate entries, and checks each entry's params.csvTimelock serializes to its own timelockToSequence value. Remove as any from isDiscoverable(DefaultContractHandler) and DefaultContractHandler.discoverAt(...) — the exported const is already typed ContractHandler<...> & Discoverable so both call sites type-check without casts. Keep as any only on the intentionally partial mock objects (mockIndexer, onchainProvider stubs).
…st import
- Rename local `lastUsedIdx` → `lastIndexUsed` in `scanContracts` so the
return statement reads `return { lastIndexUsed, handlerErrors }` with no
aliasing double-take.
- Tighten `let found;` to `let found: DiscoveredContract[];` (catch does
`continue`, so it is always assigned before use; removes the inferred
`| undefined`). Add `DiscoveredContract` to the `./types` import.
- Update `ScanContractsOptions.hd` JSDoc: false branch now reads
"probe only index 0 (single static pass)" for precision.
- Remove the unused `import { ContractManager }` from `test/restore.test.ts`;
only `makeManagerForTest()` / `makeDeps` from the helper are used in code.
- dispose() now awaits _restoreInFlight?.catch(() => undefined) before touching the contract/vtxo managers, preventing _runRestore from calling manager.refreshVtxos() or manager.scanContracts() on a torn- down manager. _runRestore never calls dispose(), so this is deadlock-free. - staticDescriptor in _runRestore is now computed lazily (only in the non-HD branch), avoiding an unnecessary xOnlyPublicKey() derivation for HD wallets where the static descriptor is never used. - JSDoc on restore() notes that concurrent calls coalesce and that a second caller's gapLimit is ignored while a restore is in flight. - Regression test: wallet.restore() + immediate wallet.dispose() (without awaiting restore first) must resolve dispose without throwing, and the restore promise must settle without an unhandled rejection.
Surface Discoverable, DiscoveryDeps, DiscoveredContract, isDiscoverable (from src/contracts/types via src/contracts/index.ts) and ScanResult, ScanContractsOptions, HandlerError (from contractManager) through the package root src/index.ts, following the existing curated import/export pattern for contract types. Cleanups from task review: - Remove spurious await on synchronous materializeDescriptorAt calls in test/restore.test.ts (method returns string, not Promise<string>). - Add inline comment above the /\/(\d+)\)\s*$/ regex in walletReceiveRotator.ts explaining what it captures. - Consolidate all mid-file import blocks in test/restore.test.ts into a single top-of-file block.
… test robustness)
…D check - scanContracts now caps the HD index range at SCAN_MAX_INDEX (10_000) instead of POSITIVE_INFINITY. Hitting the ceiling without closing the gap window throws — a buggy/malicious Discoverable handler returning unconditional hits would otherwise hang the wallet, and silently truncating a fund-recovery scan would mask the failure. - _runRestore detects HD via instanceof HDDescriptorProvider instead of duck-typing materializeDescriptorAt/advanceLastIndexUsed. The previous structural check would mis-classify any non-HD provider that happened to expose those method names. Addresses arkanaai review on PR #492.
Each scanContracts hit was routed through createContract, which calls
fetchContractVxosFromIndexer([contract]) per contract. Wallet.restore
then follows up with a single bulk refreshVtxos({ includeInactive: true })
that already covers the same scripts in one batched indexer call — making
the per-contract pulls redundant work proportional to scan hits.
Factor the validate + dedupe + persist core into upsertContract, and
add a lightweight persistAndWatchContract that omits the indexer fetch.
scanContracts now uses the lighter method; createContract keeps the
fetch for standalone callers that don't have a trailing refreshVtxos
to lean on.
The docstring already promises that a second caller's gapLimit is ignored while a restore is in flight, but the validation guard fired before the in-flight check — so a coalescing caller with an invalid gapLimit would throw instead of joining the running scan as documented. Move the _restoreInFlight check first; only the caller that actually starts the run gets its gapLimit validated. Error message unchanged.
pietro909
force-pushed
the
feat/wallet-restore
branch
from
19a0e67 to
fb8cfab
Compare
There was a problem hiding this comment.
Iterative review — commits 400829f through fb8cfab
Both open concerns from my previous review are now resolved:
✅ Unbounded loop → bounded at SCAN_MAX_INDEX = 10_000
contractManager.ts:38 — hard ceiling with a clear safety-valve comment. The post-loop check (i > maxIdx && unused < gapLimit) correctly distinguishes "gap closed naturally" from "hit the ceiling" and throws loudly for the latter. Good.
✅ Duck-typing → instanceof HDDescriptorProvider
wallet.ts:~1105 — const hd = provider instanceof HDDescriptorProvider. Comment documents the trade-off if custom HD providers are added later. Clean.
✅ New: upsertContract / persistAndWatchContract refactor (commit 3d15d5e3)
Well-factored split. persistAndWatchContract skips the per-contract fetchContractVxosFromIndexer round-trip, reducing N+1 indexer calls to 1 bulk refreshVtxos({ includeInactive: true }). The shared upsertContract preserves the existing dedupe-by-script and type-mismatch semantics. No behavioral change for external createContract callers.
✅ New: Concurrent coalesce reorder (commit fb8cfabc)
restore() now checks _restoreInFlight before validating gapLimit. Correct — a coalescing caller's value is documented as ignored, so validating it first would surface misleading errors.
✅ New: advanceLastIndexUsed input validation (commit 400829f8)
hdDescriptorProvider.ts:~140 — if (!Number.isInteger(index) || index < 0) return; silently no-ops on invalid input instead of corrupting lastIndexUsed. The monotonic guard (index > settings.lastIndexUsed) was already present; the new guard prevents NaN/fractions/negatives from bypassing it.
Test coverage
772-line unit suite + load-bearing e2e test. Coverage includes: gap-limit validation, swap-keeps-gap-open with full post-hit probe-window assertion (calls === [0..9]), per-handler error collection vs fatal propagation, static single-pass, concurrent coalesce, dispose-races-restore, HD watermark advance, partial-timelock-hit, per-script mock indexer. Comprehensive.
Protocol-critical flag
This PR touches fund recovery — a wallet.restore() that silently truncates or misidentifies scripts risks hiding user funds. The code is correct and well-tested, but human sign-off is required before merge per protocol-critical review policy.
🤖 Arkana
2 participants
Summary
Adds an explicit, mode-aware, modular
wallet.restore()that recovers a wallet's contracts and balance on a fresh device / wiped repo — closing the gap with go-sdk (gap-limit HD scan) and dotnet-sdk (modular handler discovery).wallet.restore(opts?: { gapLimit?: number }): Promise<void>— never automatic at boot.gapLimitdefaults to 20;<= 0throws.materializeDescriptorAtandadvanceLastIndexUsedso a partial customDescriptorProvidersafely takes the static path.Discoverablecapability interface onContractHandler.defaultanddelegatehandlers implement it; an external boltz/swap package can register a handler that implements it too (it closes over its own Boltz client) — core never imports boltz. Swap hits participate in the same gap-limit loop, so a swap found at HD indexikeeps the gap window open and advances the HD watermark.refreshVtxos({ includeInactive: true })sogetBalance()is correct the instantrestore()resolves.discoverAterrors are collected, not thrown — the scan finishes, the inline pull recovers safely-discovered funds, then anAggregateErroris thrown listing the failed handlers. Fatal/structural errors (indexer unreachable, descriptor materialization failure) propagate immediately so a truncated restore is never silent.createContractdedupes on script), concurrent calls coalesce, monotonic + input-validated HD watermark,dispose()drains an in-flight restore.Design
Full design + rationale (Sections 1–5, brainstormed and approved before implementation) lives in
docs/superpowers/specs/2026-05-16-restore-mechanism-design.md. Note:docs/is gitignored in this repo, so the spec/plan are local working artifacts and not part of this diff — the behavior they describe is what's implemented and tested here.Key changes
src/contracts/types.ts—Discoverable,DiscoveryDeps,DiscoveredContract,isDiscoverable.src/contracts/contractManager.ts—scanContracts()gap-limit loop +ScanResult/ScanContractsOptions/HandlerError;IContractManagerextended.src/contracts/handlers/{default,delegate}.ts— implementdiscoverAt.src/contracts/metadata.ts(new) —WALLET_RECEIVE_SOURCEextracted here to break acontracts → walletimport cycle; re-exported fromwalletReceiveRotatorfor back-compat.src/wallet/hdDescriptorProvider.ts— public purematerializeDescriptorAt; monotonic, input-validatedadvanceLastIndexUsed.src/wallet/walletReceiveRotator.ts—pickActiveReceivedeterministic tiebreak on HD index (so a tight-loop restore doesn't re-advertise an old address);signingDescriptorIndexhelper; reuses the extractedderiveDescriptorLeafPubKey.src/identity/descriptor.ts— exportedderiveDescriptorLeafPubKey(extracted from the rotator; layering fix).src/wallet/wallet.ts—Wallet.restore()+_runRestore+ in-flight guard;dispose()drains it.src/index.ts/src/contracts/index.ts— public exports for the new types.Test plan
test/restore.test.ts): helper extraction; pure/monotonic/input-validated HD provider methods;isDiscoverable; default & delegatediscoverAt(untagged index-0 vs tagged index>0, multi-timelock, partial-timelock-hit, no-delegatePubKey);scanContracts(gapLimit validation, swap-keeps-gap-open asserting the full post-hit probe window, per-handler error collected vsmaterialize-throw fatal, static single-pass);signingDescriptorIndextiebreak;Wallet.restore()(invalid gapLimit, static recovery, HD watermark advance, concurrent coalesce, handler-error rejects after the inline pull, dispose-races-restore). Full unit suite green (1223 passed, 1 pre-existing skip).test/e2e/restore.test.ts): HD-mode, load-bearing — fund Wallet A at index-0 (rotator advances to index-1), fund again at index-1, then a fresh-repo Wallet B on the same seed whose index-0 baseline cannot cover the index-1 funds →B.restore()recovers the full balance via the gap scan. Wallets disposed infinally. Runs (and passes) in CI viapnpm test:integration-dockeragainst the regtest stack.Known limitations / follow-ups
Wallet, so it does not inheritrestore(). The SW contract-manager proxy explicitly rejectsscanContracts(itsmaterializecallback is not structured-cloneable across the worker message boundary). Service-worker restore is out of scope for this change — follow-up.createContract()called per discovered contract triggers a per-contract indexer fetch, redundant with the final batchedrefreshVtxos({ includeInactive: true }). Correctness is unaffected (the final pull guarantees balance); this is a bounded perf cost on a rare explicit op. A watch-only persistence path is deferred to a follow-up rather than adding a new API to protocol-critical fund-recovery code in this PR.Review notes
CodeRabbit feedback addressed (HD capability check now requires both HD methods;
advanceLastIndexUsedinput-validated; test robustness: full post-hit probe-window assertion, per-script mock indexer + partial-hit coverage, guaranteed e2e disposal, unique mock outpoints, guarded test cast). The two items above are deliberate, reasoned deferrals. Thearkana/policyCHANGES_REQUESTEDis a protocol-critical human-sign-off gate, not outstanding defects.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes